Conversation
c202c23 to
5c9f1d0
Compare
|
timer.txt ✅: timer'txt'
|
|
testing is complete, this is ready for Sci/Tech review @tommbendall |
tommbendall
left a comment
There was a problem hiding this comment.
Thanks for doing this @mo-marqh
I spotted one unclosed timer and a typo (that wasn't yours). In the timer.txt that you posted, I noticed several 'control_*' calipers appear in gungho_transport_control. Could you also remove these?
Other than that should be good to go.
| if (du_tot_skeb_flag) call du_tot_skeb%write_field('stochastic__du_tot_skeb') | ||
| if (dv_tot_skeb_flag) call dv_tot_skeb%write_field('stochastic__dv_tot_skeb') | ||
| end if | ||
| if ( LPROF ) call start_timing( id_diags, 'diags.skeb' ) |
There was a problem hiding this comment.
| if ( LPROF ) call start_timing( id_diags, 'diags.skeb' ) | |
| if ( LPROF ) call stop_timing( id_diags, 'diags.skeb' ) |
| if ( LPROF ) call start_timing( id_skeb, 'skeb.ndisp' ) | ||
|
|
||
| !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! | ||
| !! 1) Initialize feilds and operators !! |
There was a problem hiding this comment.
| !! 1) Initialize feilds and operators !! | |
| !! 1) Initialize fields and operators !! |
Just because I spotted this typo while scrolling through!
There was a problem hiding this comment.
Can we remove all of the 'control_*' timers from this file?
|
thanks @tommbendall please confirm whether this is now okay by you? developer tests ran and passed Test Suite Results - lfric_apps - calipersPerformance25/run7Suite Information
Task Information✅ succeeded tasks - 1106 |
tommbendall
left a comment
There was a problem hiding this comment.
Thanks again for doing this. I'm very happy to have these organised nicely.
This passes science review, so over to @oakleybrunt
|
I am currently doing this review and noticed that there is quite a lot of inconsistent use of the Since the Taking the This is just my view and I am open to discussing whether this is a convention we would like to follow or not. So please reply to this with your views on this. |
the naming of this integer value is just within the scope of each code section that is calling one or more timers. I'm not keen to rename further timer_index integers in code committed in #80 within the scope of this PR, which is already large. I feel like the timer names are what should be searchable, and that using the tik integers as search items will always be fragile. I do understand your desire for consistency @oakleybrunt , i just fear that it'll be difficult to deliver in this PR, and then hard to maintain. |
Thanks Mark, I see your perspective and perhaps trying to define a convention as part of this PR is too ambitious. I think that I will start a wider discussion on naming conventions for timer hash ids instead. |
i think that consistency in the string identity naming for each timer is more useful & important. |
oakleybrunt
left a comment
There was a problem hiding this comment.
As discussed, naming conventions for hash argument not covered in this PR. Otherwise, changes are good. Thanks - approved.
resolved |
PR Summary
Sci/Tech Reviewer: @tommbendall
Code Reviewer: @oakleybrunt
Comprehensive timing caliper location review.
This is a rework of work done for performance analyses in late 2025, migrating everything caliper related from:
https://code.metoffice.gov.uk/trac/lfric_apps/browser/main/branches/pkg/Share/r15393_kpi_benchmark
essentially from this commit:
https://code.metoffice.gov.uk/trac/lfric_apps/changeset?reponame=&new=15498%40main%2Fbranches%2…
but updated from source migration and adoption of
timingwrapper from #80Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - calipersPerformance25/run6
Suite Information
Task Information
✅ succeeded tasks - 1456
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review